support custom lookup function in request() #308
Conversation
|
The windows test don't pass but I don't think that's related to my changes? |
There was a problem hiding this comment.
Thanks for the quite nice PR submission.
I am not sure I see the need to handle the lookup option explicitly when it can be done with custom logic in the preRequest hook. This seems like a quite esoteric use case.
If we add this, then we should probably also support the family and hints options to allow all name resolution related options.
If we really want to go for this, I would probably rework the implementation to create an internal list of directly copied options, and iterate over this when copying the options over. Eg. ['secureProtocol', 'ciphers', 'lookup', 'family', 'hints'].
API.md
Outdated
| - `lookup` - DNS lookup function. see [http.request(url[,options][,callback])](https://nodejs.org/api/http.html#httprequestoptions-callback). Defaults to [dns.lookup()](https://nodejs.org/api/dns.html#dnslookuphostname-options-callback). | ||
|
|
There was a problem hiding this comment.
This belongs in the request() section.
test/index.js
Outdated
| } | ||
| }); | ||
| expect(dnsLookupCalled).to.equal(true); | ||
| flags.onCleanup = () => server.close(); |
There was a problem hiding this comment.
The onCleanup should be registered right after the server is created.
I see that this copies behaviour from other tests. Don't ask me why this has been done previously.
lib/index.js
Outdated
| @@ -61,6 +61,7 @@ internals.Client = class { | |||
| Hoek.assert(internals.isNullOrUndefined(options.beforeRedirect) || typeof options.beforeRedirect === 'function', 'options.beforeRedirect must be a function'); | |||
| Hoek.assert(internals.isNullOrUndefined(options.redirected) || typeof options.redirected === 'function', 'options.redirected must be a function'); | |||
| Hoek.assert(options.gunzip === undefined || typeof options.gunzip === 'boolean' || options.gunzip === 'force', 'options.gunzip must be a boolean or "force"'); | |||
| Hoek.assert(options.lookup === undefined || typeof options.lookup === 'function', 'options.lookup must be a function'); | |||
There was a problem hiding this comment.
I'm not sure we actually need to assert() this. We only check wreck specific options as it is.
|
Thanks for reviewing my PR!
Makes sense, I've adapted the code to reflect your suggestions. Regarding the use-case, I'd argue having user-controlled URLs called by servers is not esoteric, and servers are probably more often than not vulnerable to SSRFs (Server Side Request Forgery). Having this option built-in makes one step towards a better support for preventing access to the internal network of a server |
|
Nothing to add to what Gil said, thanks for your contribution, it's shipped! |
Description
Support custom DNS lookup function. This is particularly useful to prevent user-controlled requests accessing internal services, by denying accessing requests where the resolved IP is not public.
Alternatives
This could also be implemented by using a custom
preRequesthook, but directly supporting the Http.requestlookupparameter is the most straightforward way to pass options to a specific request.I've implemented tests and documented the parameter both in API.md and in the index.d.ts file, please tell me if there are other parts that I've missed! I didn't touch the package version out of caution.
Happy to discuss it.